The change looks good and makes sense to me! 👍
I think we need more tests to ensure implementation compliance

Sounds logical... good writeup

I think we need more tests to ensure implementation compliance

It shouldn't be necessary to add extra tests. There should be several sanity tests which will check this.

For example, test_proposer_after_inactive_index and test_high_proposer_index would most likely error if get_beacon_proposer_index were implemented incorrectly.

• consensus-specs/tests/core/pyspec/eth2spec/test/phase0/sanity/test_blocks.py

  Lines 669 to 716 in 554189d

  	@with_all_phases
  	@spec_state_test
  	def test_proposer_after_inactive_index(spec, state):
  	# disable some low validator index to check after for
  	inactive_index = 10
  	state.validators[inactive_index].exit_epoch = spec.get_current_epoch(state)
  	# skip forward, get brand new proposers
  	next_epoch_via_block(spec, state)
  	next_epoch_via_block(spec, state)
  	while True:
  	proposer_index = spec.get_beacon_proposer_index(state)
  	if proposer_index > inactive_index:
  	# found a proposer that has a higher index than a disabled validator
  	yield 'pre', state
  	# test if the proposer can be recognized correctly after the inactive validator
  	signed_block = state_transition_and_sign_block(spec, state, build_empty_block_for_next_slot(spec, state))
  	yield 'blocks', [signed_block]
  	yield 'post', state
  	break
  	next_slot(spec, state)
  	@with_all_phases
  	@spec_state_test
  	def test_high_proposer_index(spec, state):
  	# disable a good amount of validators to make the active count lower, for a faster test
  	current_epoch = spec.get_current_epoch(state)
  	for i in range(len(state.validators) // 3):
  	state.validators[i].exit_epoch = current_epoch
  	# skip forward, get brand new proposers
  	state.slot = spec.SLOTS_PER_EPOCH * 2
  	block = build_empty_block_for_next_slot(spec, state)
  	state_transition_and_sign_block(spec, state, block)
  	active_count = len(spec.get_active_validator_indices(state, current_epoch))
  	while True:
  	proposer_index = spec.get_beacon_proposer_index(state)
  	if proposer_index >= active_count:
  	# found a proposer that has a higher index than the active validator count
  	yield 'pre', state
  	# test if the proposer can be recognized correctly, even while it has a high index.
  	signed_block = state_transition_and_sign_block(spec, state, build_empty_block_for_next_slot(spec, state))
  	yield 'blocks', [signed_block]
  	yield 'post', state
  	break
  	next_slot(spec, state)

we should verify there is some difference in test output

Good call. I have a simple difference. In test_high_proposer_index, the state's slot will be different. This is because the following check will require more iterations.

See slot: 17 in the first test run (on dev) and slot: 20 on the second one (on this branch):